fix(policy): keep approved chunk when a mechanistic denial resubmits its endpoint#2242
Conversation
…its endpoint A mechanistic denial flush for an endpoint already covered by an auto-approved mechanistic chunk flipped that chunk approved -> rejected with no human action. The dedup upsert in put_draft_chunk returns the existing row's id, which aliases onto the approved chunk; the self-reject scan then matched the row against itself and rejected it, while the merged rule stayed enforced — the governance ledger disagreed with the live policy. Guard self_reject_mechanistic_if_already_covered to act only on a still pending effective chunk, and exclude the incoming id from the covering scan. Add a regression test and document the dedup/self-reject invariant. Fixes #2165 Refs NVIDIA/NemoClaw#6329 Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
TaylorMutch
left a comment
TaylorMutch
left a comment
There was a problem hiding this comment.
The deterministic self-aliasing fix and handler-level regression test are sound, but the new pending-status guard does not yet guarantee the invariant documented by this PR.
In self_reject_mechanistic_if_already_covered, the code reads the chunk and confirms that it is pending, then later calls the existing unconditional update_draft_chunk_status(..., "rejected", ...). An approval can race between those operations, allowing an approved chunk to be changed to rejected while its rule remains merged. That recreates the same ledger/enforcement mismatch through a concurrent path.
Please make pending -> rejected an atomic compare-and-set in the policy store and implement it for both SQLite and PostgreSQL. The final database write should include status = 'pending' in its predicate and return whether a row changed; zero changed rows should be treated as a benign no-op because another operation already decided the chunk. Since the persisted payload mirrors the status and decision metadata, construct and write that payload transactionally or otherwise retain the status predicate on the final update.
Please also add a persistence-level regression test proving that an approved row cannot be conditionally rejected. Keep the c.id != new_chunk_id exclusion as defense-in-depth.
johntmyers
added
the
gator:in-review
PR Review StatusValidation: This is a project-valid, focused fix for the confirmed policy-governance defect tracked in #2165. Review findings:
Thanks @TaylorMutch. I checked the atomic-transition concern you raised on #2165 against the self-reject, supersede, direct approval/rejection, and bulk approval call paths, as well as both production persistence backends; the concern is confirmed and remains blocking on this head. What looks good: excluding Docs: no Fern update is needed because this restores existing internal policy behavior; the relevant architecture documentation is updated. Next state: |
The self-reject-when-covered path read a chunk, confirmed it was pending, then issued an unconditional status update to rejected. An approval that committed between the read and the write flipped an already-approved chunk to rejected while its rule stayed merged, recreating the ledger/enforcement mismatch through a concurrent path. Add conditionally_reject_draft_chunk to the policy store: the pending->rejected transition carries a status = 'pending' predicate on the final write and reports whether a row changed. Zero changed rows is a benign no-op, meaning another operation already decided the chunk. Implemented for both SQLite and PostgreSQL. The pending pre-read and the self-exclusion guard stay as defense-in-depth. Adds persistence-level regression tests proving an approved row cannot be conditionally rejected and that an approval racing the reject wins. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Label |
johntmyers
added
gator:watch-pipeline
PR Review StatusValidation: This remains a project-valid, focused fix for the confirmed policy-governance defect tracked in #2165. Review findings:
Thanks @TaylorMutch. I checked the atomic-transition concern you raised against the current self-reject flow and both persistence backends; this head resolves it for the defect in scope. Docs: no Fern update is needed because this restores intended internal policy behavior without changing commands, configuration, API contracts, or user workflows. The architecture documentation is updated appropriately. E2E: Next state: |
johntmyers
added
gator:approval-needed
Monitoring CompleteMonitoring is complete because this PR has merged. Final status: head I removed the active |
johntmyers
removed
the
gator:approval-needed
3 participants
Summary
A mechanistic denial flush for an endpoint already covered by an auto-approved mechanistic chunk flipped that chunk from
approvedtorejectedwith no human action, corrupting the governance ledger: the self-reject path never un-merges the rule, so the endpoint stayed enforced while the ledger reported it revoked (and the chunk became permanently stuck, since dedup resubmits land on arejectedrow and auto-approval only fires frompending). The root cause is id aliasing —put_draft_chunk's dedup upsert returns the existing approved row's id, which the self-reject scan then matched against itself.The same ledger/enforcement mismatch was also reachable through a concurrent path: the self-reject read a chunk, confirmed it was
pending, then issued an unconditional status update, so an approval that committed between the read and the write flipped the approved chunk torejected. That window is now closed by making thepending -> rejectedtransition an atomic compare-and-set in the policy store.Related Issue
Fixes #2165
Refs NVIDIA/NemoClaw#6329
Changes
self_reject_mechanistic_if_already_coverednow only acts on a still-pendingeffective chunk: it re-fetches the chunk the dedup upsert resolved to and returns early unless its status ispending, so a dedup hit that aliases onto an already-decided (approved) row can never flip it.c.id != new_chunk_id), so an aliased row cannot match itself as its own "covering approved chunk".conditionally_reject_draft_chunkstore method performs thepending -> rejectedtransition as an atomic compare-and-set: thestatus = 'pending'predicate rides the final write and the call reports whether a row changed. Zero changed rows is a benign no-op, meaning another operation already decided the chunk. Implemented for both SQLite and PostgreSQL. The self-reject path now commits through it, so an approval racing the reject can no longer clobber an approved row; thependingpre-read and the self-exclusion stay as defense-in-depth.resubmitted_mechanistic_endpoint_keeps_approved_chunk: auto-approve a mechanistic endpoint, resubmit the same endpoint mechanistically, and assert the chunk staysapproved,hit_countincrements, norejection_reasonis set, and the rule remains merged in the active policy.conditionally_reject_transitions_pending_chunk(a pending chunk is rejected),conditionally_reject_leaves_approved_chunk_untouched(an approved row cannot be conditionally rejected), andconditionally_reject_loses_race_to_approval(an approval that commits before the reject wins).architecture/security-policy.md(Policy Advisor pipeline).Testing
mise run pre-commitpassesChecklist